Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

introduce two CDI helper extension to add beans explicitly #2125

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

introduce two CDI helper extension to add beans explicitly #2125

wants to merge 1 commit into from

Conversation

craig-day
Copy link

@craig-day craig-day commented Mar 29, 2023 •
edited
Loading

As discussed in #2115, this introduces some CDI extensions to act as a bridge for environments where auto bean discovery is not utilized. A consumer can chose to explicitly add these extensions to their CDI container to get the same support that auto discovery would have provided.

Opening as a draft to start to see how we feel about the approach before propagating it to all other connector implementations and updating tests.

@craig-day craig-day mentioned this pull request Mar 29, 2023
Open
@manovotn
Copy link
Contributor

Few comments coming to my mind.

  1. From CDI perspective, this form of bean registration seems fine 👍
    However, I'd add a comment into each extension denoting that these CDI extensions are disabled by default as they intentionally have no META-INF entry in this repository and that they are meant to be used explicitly by integrators.

  2. As for the set of classes added, I have no idea which classes SR reactive messaging need so I'll trust you know that.
    How likely is this set going to change or expand? Since this way it would be hard to remember to add these beans into extensions as well.

  3. I think the tests using Weld SE should leverage these extensions - i.e. the base test class should register these extensions (whichever are needed) instead of manually registering the beans. That will also help cover the case of newly added CDI beans.

  4. Last but not least, we should double check that all modules that should have beans.xml (and are subject to having the extension added) do have one because adding an extension into them might otherwise trigger "no archive behavior" as per this spec part:

An archive which:

  • contains a beans.xml file with the bean-discovery-mode of none, or,
  • contains a portable extension or a build compatible extension and no beans.xml file

is not a bean archive.

@ozangunalp
Copy link
Collaborator

I am not against this change but we'll need to find some distinctive names for these "unexposed" CDI Extension classes.
ReactiveMessagingCDIExtension is too similar to ReactiveMessagingExtension.

As @manovotn mentioned we should register these extensions when running the connector tests. It'll also serve as a way to check that the Extension is not missing a critical bean.
For example change the following addBeanClass with addExtension in https://github.com/smallrye/smallrye-reactive-messaging/blob/main/smallrye-reactive-messaging-kafka/src/test/java/io/smallrye/reactive/messaging/kafka/base/WeldTestBase.java#L69

We would do the same for connectors other than Kafka too. I think we have beans.xml classes for every connector module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@craig-day @manovotn @ozangunalp